Jira URL: https://ixsystems.atlassian.net/browse/NAS-141049

Claude finished @AlexKarpov98's task in 7m 19s —— View job

Review

This is an unusually disciplined migration — the centralized "Known upstream defects" table in SKILL.md, the shared tnSelectLabels / optionTestIdBy* constants, translatedSignal(), and the TnFormControlHarness adapter that lets half-migrated forms still be driven by label are all the right level of abstraction, and every workaround carries a TEMP (NAS-141021) marker with a drop condition. 🎯 The a11y wins are real too: the clickable <div> in manual-selection-vdev and the <a tabindex="0"> pseudo-links in storage-health-card became real <button>s, and ix-tn-radio-group finally gives the radios a named role="radiogroup".

A few things worth another look:

1. dRAID selects can display a value that isn't in their own option list

draid-selection.component.ts guards only children against this:

// draid-selection.component.ts:230
if (this.isStepActive() && maxPossibleWidth) {

but dataDevicesPerGroup (default 1), spares (0) and vdevsNumber (1) keep their declared defaults through the reset at draid-selection.component.ts:112. With no disks selected their option lists are empty, so the user sees a trigger reading 1 over a dropdown that says "No options" — ix-select used to blank exactly that state. The reasoning in the comment ("a reset form is indistinguishable from a freshly constructed one") is sound for the control value, but a select whose visible text isn't one of its options is a real oddity. Extending unsetControlIfNoMatchingOption to these three (or gating the whole dRAID panel on a disk size being picked) would close it.

Fix this →

2. [inlineFields]="true" was dropped from the pool-warnings radio group

Pre-migration (pool-warnings.component.html) the "Allow non-unique serialed disks" group rendered its Yes/No options in a row. ix-tn-radio-group's SCSS is just :host { display: block }, the template's wrapper is a plain <div role="radiogroup">, and the component's ix-radio-group { margin-top: 0 } rule was deleted with nothing laying the options out horizontally. Unless tn-radio's own host is inline, that two-option group now stacks. Worth a visual check, and if it does stack, an opt-in inline input on ix-tn-radio-group is a better home for it than a page-level override.

3. TnRadioGroupComponent's name uniqueness is documented but not enforced

The doc comment is explicit that a duplicated native name fuses two groups and breaks arrow-key navigation, and that a caller rendering the group inside a @for must "derive a discriminator into the name". But name is never surfaced to the user, so nothing is lost by making the component do that itself — append a per-instance counter to the value passed to <tn-radio [name]> and the footgun disappears for every future consumer. All three current call sites render once, so this is cheap insurance rather than a bug today.

Fix this →

4. New shared test infrastructure has no spec of its own

tn-form-control.harness.ts is now the adapter every half-migrated form's IxFormHarness-style lookup flows through, and it carries genuinely tricky logic: the white-box .tn-select-text.placeholder probe, the radio fan-out in getValue/setValue, the "every radio disabled" reduction in isDisabled, and the checkbox-self-label fallback in getLabelText. Codecov flags 18 uncovered lines there. Given how many specs now depend on it, a small direct spec (one tn-form-field per supported control + the unsupported-control throw) would be cheap and would pin the contract.

Related, and worth at least a comment: getValue() returning ''/false for an unsupported control means a getControlValues() assertion can pass while reading nothing. The rationale (whole-form readers must not be taken down by one control) is fair, but setValue's explicit throw shows the alternative was available — a sentinel that a whole-form reader skips would give you both.

5. The dropped-'' index entry produces a misleading error

indexControlsByLabel deleting the ambiguous '' key is the right call, but the failure a caller then sees is fillControlValues' Could not find control with label . — which reads like a typo rather than "you have two unlabelled controls, query them directly." Threading the reason through (or having fillControlValues special-case the empty label) would save the next person the trip through this file.

6. Small scope nits, in a PR that's already tightening scope

You correctly moved a pile of members to private/protected in dashboard-pool, vdevs-card and pools-dashboard — a couple of neighbours got missed:

• scrub-form.component.ts:63 — get isNew() is only read at line 113; should be private.
• import-pool.component.ts:72 — importablePools is public but only used internally; helptext is public but only used from the template (protected).

Not blocking, just noting: the (x$ | async) ?? [] pattern now appears in ~8 templates purely to feed tn-select [options] an array. import-pool shows the nicer end state (poolOptions = signal<Option[]>([])); converting the rest is a fine follow-up rather than something to grow this diff for.

⚠️ I couldn't run yarn lint or the specs in this environment (no node_modules, and the install step isn't permitted here), so this review is static-analysis only — please rely on CI for the spec results.
· branch NAS-141049

Codecov Report

❌ Patch coverage is 91.92201% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.66%. Comparing base (a5610f7) to head (3a97c93).
⚠️ Report is 7 commits behind head on master.

@@            Coverage Diff             @@
##           master   #13868      +/-   ##
==========================================
+ Coverage   87.63%   87.66%   +0.03%     
==========================================
  Files        1895     1901       +6     
  Lines       71860    72036     +176     
  Branches     9142     9187      +45     
==========================================
+ Hits        62972    63148     +176     
  Misses       8888     8888              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.